Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make zenoh router configurable using envar #119

Merged
merged 10 commits into from
Feb 28, 2024

Conversation

Yadunund
Copy link
Member

This PR

  • Allows users to specify a custom the router configuration file via the ZENOH_ROUTER_CONFIG_URI envar.
  • The envar to specify the session config file is now ZENOH_SESSION_CONFIG_URI
  • The router executable is now zenohd to align with that of upstream Zenoh.
  • Readme updated to inform users how to configure these files.

Addresses #102

Copy link
Collaborator

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't do a full review here, but I did want to discuss this point:

The router executable is now zenohd to align with that of upstream Zenoh.

I'm not so sure that is a great idea. The fact is that while this is similar to the upstream zenohd, it is not exactly the same and doesn't support all of the same things (in particular, plugins). So I do think we should make it clear that this is the rmw_zenoh_cpp version of this. I know that ros2 run rmw_zenoh_cpp zenohd does that, but:

  1. You don't have to run it that way; you can also do /path/to/zenohd, and then it looks just like the upstream one.
  2. In the process listing, it will show up as zenohd. Yes, the full executable path will be to an rmw_zenoh-specific path, but it is an easy detail to overlook.

My suggestion is that we do indeed rename it, but to something else. Something like rmw_zenohd or something like?

@Yadunund
Copy link
Member Author

I didn't do a full review here, but I did want to discuss this point:

The router executable is now zenohd to align with that of upstream Zenoh.

I'm not so sure that is a great idea. The fact is that while this is similar to the upstream zenohd, it is not exactly the same and doesn't support all of the same things (in particular, plugins). So I do think we should make it clear that this is the rmw_zenoh_cpp version of this. I know that ros2 run rmw_zenoh_cpp zenohd does that, but:

  1. You don't have to run it that way; you can also do /path/to/zenohd, and then it looks just like the upstream one.
  2. In the process listing, it will show up as zenohd. Yes, the full executable path will be to an rmw_zenoh-specific path, but it is an easy detail to overlook.

My suggestion is that we do indeed rename it, but to something else. Something like rmw_zenohd or something like?

That makes a lot of sense to me. I've renamed the executable to rmw_zenohd 👍🏼

Copy link
Collaborator

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a few pretty minor things to change. Otherwise, this looks good to me.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Comment on lines 33 to 42
///==============================================================================
/// Map the configurable entity to a pair of environment variable name that
/// stores the absolute path to the Zenoh config and the default config filename.
/// Note: The default config file should be located within rmw_zenoh_cpp/config/.
static const std::unordered_map<ConfigurableEntity,
std::pair<const char *, const char *>> envar_map = {
{ConfigurableEntity::Session,
{"ZENOH_SESSION_CONFIG_URI", "DEFAULT_RMW_ZENOH_SESSION_CONFIG.json5"}},
{ConfigurableEntity::Router, {"ZENOH_ROUTER_CONFIG_URI", "DEFAULT_RMW_ZENOH_ROUTER_CONFIG.json5"}}
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this map can be put into zenoh_config.cpp. That will have the big benefit of it not being included with every compilation unit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yadunund and others added 5 commits February 28, 2024 21:23
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Yadu <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Yadu <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Yadu <[email protected]>
Signed-off-by: Yadunund <[email protected]>
@clalancette clalancette merged commit 8db8003 into rolling Feb 28, 2024
5 checks passed
@delete-merged-branch delete-merged-branch bot deleted the yadu/configurable_router_config branch February 28, 2024 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants